Drop wheel, setuptools and pytest from runtime dependencies - #320
Drop wheel, setuptools and pytest from runtime dependencies#320fwittreverce wants to merge 3 commits into
Conversation
None of the three is imported anywhere under `src/`. `c2pa.py` and `lib.py` import only the standard library; `build.py` — the `download-artifacts` console script — imports `requests` and, lazily, `toml`. Those two stay. They are also already classified correctly elsewhere in the repo: * `[build-system] requires` already lists `setuptools>=68.0.0` and `wheel`, so the build has what it needs and the runtime entries are duplicates. * `requirements-dev.txt` lists `wheel` and `setuptools` under "# Build dependencies" and `pytest` under "# Testing dependencies". * `.github/workflows/build.yml` installs pytest explicitly (`pip install pytest`, lines 285 and 377), so CI does not rely on the runtime declaration either. Removing them is therefore a no-op for this repo's own build and test paths, and it keeps three packages out of every consumer's production environment.
|
For further review and consideration, please make sure to sign the Adobe CLA. You will likely need to close and reopen the PR for the job to pass. |
Dropping pytest from `[project.dependencies]` left it undeclared in pyproject.toml entirely, with `requirements-dev.txt` as the only manifest naming it. `[dependency-groups] dev` states it where it belongs: installed for contributors (`uv sync`, `pip install --group dev`) and, unlike `[project.optional-dependencies]`, absent from the published package metadata — which is the separation this branch is about. The bound matches requirements-dev.txt (`pytest>=8.1.0`) rather than the `>=7.4.0` the runtime entry carried; nothing installs the old one. The comment above the remaining dependencies goes with it. The rationale for keeping `toml` and `requests` belongs in the pull request, not in a manifest that has carried no comments so far.
|
@tmathern fixed. |
|
|
||
| [dependency-groups] | ||
| dev = [ | ||
| "pytest>=8.1.0" |
There was a problem hiding this comment.
There is a pytest in:
https://github.com/contentauth/c2pa-python/blob/main/requirements-dev.txt#L8. What do you think of completely removing it here, and keeping the one in requirements-dev.txt only, so they couldn't drift?
There was a problem hiding this comment.
Happy to — removing it outright is the smaller diff, and it makes this PR purely
subtractive. Nothing in the repo reads a [dependency-groups] entry today: the dev
path is pip install -r requirements-dev.txt everywhere (make install-deps, and
build.yml lines 52, 90/93, 164/167, 490/492). The group was defensive rather than
load-bearing — the point of the PR is the published metadata, which is fixed either
way. I'll push that.
One observation, offered rather than requested: the drift here is structural rather
than accidental, so removing my line removes one instance of it, not the mechanism.
Today pytest reads >=7.4.0 in pyproject.toml against >=8.1.0 in
requirements-dev.txt, and cryptography reads >=41.0.0 in [project.dependencies]
against >=47.0.0 in both requirements files. This PR settles the first pair; the
second stays.
If you ever wanted that impossible rather than merely absent, the standards have since
caught up with everything these files are doing:
| what it declares | standard home |
|---|---|
| published runtime deps | [project.dependencies] — already there |
| build backend deps | [build-system] requires — already there; the wheel / setuptools / toml lines in requirements-dev.txt duplicate it |
| dev sets: test, lint, docs | [dependency-groups] — PEP 735, final 2024-10-10; pip install --group <name> since pip 25.1 |
| a pinned environment | pylock.toml — PEP 751, final 2025-03-31 |
which for this repo is roughly:
[dependency-groups]
test = ["pytest>=8.1.0", "cryptography>=47.0.0"]
lint = ["autopep8>=2.3.0", "flake8==7.3.0"]
docs = ["Sphinx>=7.3.0", "sphinx-autoapi>=3.0.0", "myst-parser>=2.0.0", "furo>=2024.0.0"]
dev = [{include-group = "test"}, {include-group = "lint"}, {include-group = "docs"}]with make install-deps becoming pip install --group dev. Every bound would then
exist exactly once, and a group cannot drift from itself.
Two caveats I would rather name than have you discover: it needs pip >= 25.1 on every
path that installs, and it is not a small diff — Makefile, build.yml,
build-wheel.yml, publish-docs.yml, docs/project-contributions.md (eight
references) and the perf Dockerfiles all name the requirements files today.
requirements.txt would need a decision rather than a translation: its single line is
an example dependency that is also published as a runtime dep, which is the same
question I left open at the bottom of this PR.
Your call entirely, and clearly not this PR — I am happy to open it separately if you
want it, and equally happy to leave it be.
Review feedback: the PEP 735 group restated a bound that `requirements-dev.txt` already carries, so the two could drift — which is what this branch set out to stop, not to reproduce one line further down. Nothing in the repo would have read the group. The Makefile's `install-deps` and every workflow that installs dependencies do so with `pip install -r requirements-dev.txt` (`build.yml` lines 52, 90/93, 164/167, 490/492), and the wheel test jobs install pytest by name. The group was a declaration with no consumer. The published metadata — the point of this branch — is unaffected either way, and the diff is now purely subtractive.
2 participants
wheel,setuptoolsandpytestare declared in[project.dependencies], soevery consumer installs three build/test packages into production environments.
None of the three is imported anywhere under
src/.pytestis re-declared as a PEP 735[dependency-groups] deventry rather thandropped outright, so the manifest still names the test dependency —
uv syncand
pip install --group devpick it up — while keeping it out of the publishedpackage metadata, which
[project.optional-dependencies]would not do. Thebound follows
requirements-dev.txt(>=8.1.0) rather than the>=7.4.0theruntime entry carried; nothing installs the older one today.
Evidence
Every import in the installed package, on
main(0.37.9):src/c2pa/c2pa.pysrc/c2pa/lib.pysrc/c2pa/__init__.pysrc/c2pa/build.pyrequests(line 16),toml(line 97, lazy)So
requestsandtomlare genuine runtime dependencies —build.pyis theinstalled
download-artifactsconsole script — and this PR leaves both alone.wheel,setuptoolsandpytestare imported by nothing.This repo already classifies all three correctly in three other places:
[build-system] requiresalready listssetuptools>=68.0.0andwheel,so the build has what it needs; the runtime entries are duplicates.
requirements-dev.txtlistswheelandsetuptoolsunder# Build dependenciesandpytestunder# Testing dependencies..github/workflows/build.ymlinstalls pytest explicitly(
pip install pytest, lines 285 and 377), so CI does not rely on the runtimedeclaration either.
The change is therefore a no-op for this repo's own build and test paths.
Why it is worth doing
They reach production images. In our worker,
uv export --frozen --no-dev—the resolver's production set — lists all three.
setuptoolsin particular hasa CVE history, so a security scan has to triage findings for packages the
application never imports.
They mask missing test dependencies downstream, silently. This is the one
that cost us time. Our own test extra was never actually installed —
uv syncdoes not install extras — and nobody noticed for seven weeks, because
pytestarrived through this dependency chain anyway. Our suite ran on a package no
manifest of ours declared, at a version nobody chose, and a fix we shipped in
that window was inert the whole time. A dependency audit is what eventually
flagged it.
One question, deliberately not in this diff
cryptographyis also unimported undersrc/— it appears only inexamples/and
tests/, and your ownrequirements.txtsays# only used in the training example. It looked like your call rather than mine: dropping it would stoppip install c2pa-pythonfrom giving a reader everything the signing examplesneed. Happy to extend the PR if you would rather it went too.